docs(parity): file P9-P12 — wrong-result bugs found while surveying transformers for R2#34
Merged
Merged
Conversation
The walker migration surfaced real wrong-result bugs, not just structural
debt. Each was reproduced against the CLI and is now pinned by a corpus
case, so they cannot regress further or be silently fixed.
P9 -- HAVING silently mishandles an aggregate nested in a non-comparison
operator. HavingAliasTransformer only matches FunctionCall/BinaryOp/Not,
so an aggregate reached any other way is never rewritten to its alias.
The predicate then does not filter as written, and the result is wrong in
BOTH directions with no error raised:
HAVING COUNT(*) BETWEEN 1 AND 2 ref 1 row -> ours 4 rows (under-filters)
HAVING COUNT(*) IN (4, 5) ref 2 rows -> ours 0 rows (over-filters)
HAVING CASE WHEN COUNT(*) > 2 ... ref 2 rows -> ours 0 rows
HAVING COUNT(*) > 2 works, which is why nobody noticed. The code comment
at having_alias_transformer.rs:213 says the untransformed aggregate "will
fail later" -- it does not fail, it returns wrong rows.
P10 -- HAVING NOT (COUNT(*) > 2) errors in the arithmetic evaluator.
Distinct from P9: here the aggregate IS rewritten correctly and the gap
is downstream, so fixing P9 will not fix this.
P11 -- a SELECT alias on the LHS of an IN subquery is never expanded
("Column 'dbl' not found"). WhereAliasExpander skips the same-scope
operand along with the subquery. It is right not to descend into the
subquery body; the bug is dropping the operand too.
P12 -- WITH is rejected in every expression position tried. Found while
trying to write a regression test for the cte_hoister migration that
turned out to be inexpressible. Side effect: the ScalarSubquery /
InSubquery arms of CTEHoister::hoist_from_expression are unreachable dead
code today.
Adds corpus tier 07 (grouping). The corpus had NO HAVING coverage at all
before now, which is the direct reason P9 survived -- worth treating an
absent test bucket as a finding in its own right.
One trap worth recording: the first draft of having_in_list used
IN (2, 3), which no group satisfies, so both engines returned 0 rows and
the case passed for the wrong reason. Corrected to IN (4, 5) against the
real group counts (5/10/4/1).
Corpus grows 72 -> 83 cases; contract holds. Cross-links P9/P11 to R3 in
ENGINE_REFACTORING.md, which now carries the evidence table.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Answering "have we uncovered new P-style parity problems?" — yes, four, one of them serious. Each was reproduced against the CLI rather than inferred from reading code, and each is now pinned by a corpus case so it can't regress further or be silently fixed.
Corpus grows 72 → 83 cases; contract holds.
P9 —
HAVINGsilently returns wrong rows 🔴The one that matters.
HavingAliasTransformerrewrites an aggregate inHAVINGto reference its computed alias, but only matchesFunctionCall/BinaryOp/Not. An aggregate reached any other way is never rewritten, so the predicate doesn't filter as written — wrong in both directions, no error:HAVING COUNT(*) BETWEEN 1 AND 2HAVING COUNT(*) IN (4, 5)HAVING CASE WHEN COUNT(*) > 2 THEN 1 ELSE 0 END = 1HAVING COUNT(*) > 2works fine, which is why this went unnoticed. The code comment athaving_alias_transformer.rs:213says the untransformed aggregate "will fail later" — it doesn't fail, it returns wrong rows.Root cause is R3: a
_ => {}catch-all turned a missing match arm into a wrong answer. Will be fixed by the R2 group-2 migration.P10 —
HAVING NOT (...)errorsUnsupported expression type for arithmetic evaluation: Not { ... }. Distinct from P9 — here the aggregate is rewritten correctly and the gap is downstream in the evaluator, so fixing P9 won't fix this. Small, independent.P11 —
SELECTalias not expanded on the LHS of anINsubqueryWHERE dbl IN (SELECT ...)→Column 'dbl' not found, wheredblis a SELECT alias. Works fine as the LHS of a plain comparison or anIN-list.WhereAliasExpanderis right not to descend into the subquery body (different scope) — the bug is that it drops the same-scope operand too.walkgets this distinction right for free, so this also falls out of the group-2 migration.P12 —
WITHrejected in expression positionWHERE price > (WITH avg_cte AS (...) SELECT a FROM avg_cte)→Parse error: Unexpected token in primary expression: With. Rejected in every position tried — scalar subquery,BETWEENoperand,IN-list element, tupleIN. DuckDB accepts it.Found while trying to write a regression test for the
cte_hoistermigration that turned out to be inexpressible. Side effect worth knowing: theScalarSubquery/InSubqueryarms ofCTEHoister::hoist_from_expressionare unreachable dead code today — expression-position CTE hoisting has never had an input.The meta-finding
The corpus had no
HAVINGcoverage at all before this PR. That's the direct reason P9 survived, and it seems worth treating an absent test bucket as a finding in its own right — noted in R3 as a check to run when migrating any transformer.A trap I nearly shipped
My first draft of
having_in_listusedIN (2, 3). No group has 2 or 3 rows, so both engines correctly returned 0 rows and the case passed — a green test proving nothing. Corrected toIN (4, 5)against the real group counts (5/10/4/1), at which point it correctly reports DIFFER. The comment in the corpus records this so nobody re-introduces it.Independent of #33
None of P9–P12 are fixed by the group-1 migration, so this branches off
mainand can merge in either order. Verified the expectations hold against amainbuild, not just the group-1 build.🤖 Generated with Claude Code